Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(lavalink)!: Lavalink refactor to v4 API #2322

Open
wants to merge 70 commits into
base: main
Choose a base branch
from

Conversation

LinuxDevon
Copy link
Contributor

@LinuxDevon LinuxDevon commented Mar 14, 2024

Here are the changes required to work against the new lavalink API: https://lavalink.dev/api/index.html.

I didn't fully implement all the API since some of it is new in v4. I only maintained the compatibility of the current system that was built on v3. I put the structs / enums under deserialization and serialization tests with json i received when testing the bot and with some in the documentation. I also tested against my bot and seems to work just as well as before.

Major changes were:

  • REST API now for all outgoing events. I used a hyper http client for this.
  • Websocket is only used for connection and incoming
  • Structs and enums changed names and updated members
  • New api now versions the api in the uri /v4/...
  • V3 lavalink servers are not compatible now
  • Refactored all of the incoming and outgoing into their own files within model folder

There are a few other things i want to change but they aren't critical. I want to clean up the http since it has some model structs that could make more sense in model. The http also just generates the body and parts for the http requests for some endpoints. I think the player needs to hold the client and make these calls for the user. There is too much detail exposed in my opinion. That will be a different day and effort but wanting to jot that down now.

Please let me know what I need to change and can do those and test with the basic bot. I updated the documentation with what i thought was relevant.

This covers the issue: #2192

@github-actions github-actions bot added c-lavalink Affects the lavalink crate m-breaking change Breaks the public API. t-feature Addition of a new feature labels Mar 14, 2024
@LinuxDevon LinuxDevon marked this pull request as ready for review March 14, 2024 20:15
@vilgotf vilgotf self-requested a review March 16, 2024 06:06
Copy link
Member

@vilgotf vilgotf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove all unwraps that are not provably unreachable or document those functions as panicking on . I would also like struct fields and enum variants to be alphabetically sorted everywhere.
Lastly, if it's not too much work, it would be great to complete the Lavalink v4 support in one go.

twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
examples/lavalink-basic-bot.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/model/incoming.rs Outdated Show resolved Hide resolved
@LinuxDevon
Copy link
Contributor Author

LinuxDevon commented Mar 20, 2024

@vilgotf

Please remove all unwraps that are not provably unreachable or document those functions as panicking on . I would also like struct fields and enum variants to be alphabetically sorted everywhere.
Lastly, if it's not too much work, it would be great to complete the Lavalink v4 support in one go.

So for the panics, you just mean send them up as a result correct?

Sorry for the structs change. I was following the same order as the lavalink documentation to ensure i had all the new fields. I can revert those to alphabetical order. Didn't catch that before.

I personally don't have the time currently to finish the v4 implementation. I can maybe work on that at a later time but constrained at the moment. It will just take quite some time to verify the changes is the problem. The missing pieces are really just the extra stuff such as lavalink version, more filters (i haven't needed or used them), session info which isn't required, and i think route planning which i haven't needed either and quite frankly not sure how that all works. Unless someone else can help i won't be able to get that together.

@vilgotf
Copy link
Member

vilgotf commented Mar 22, 2024

So for the panics, you just mean send them up as a result correct?

Yes

I personally don't have the time currently to finish the v4 implementation. I can maybe work on that at a later time but constrained at the moment. It will just take quite some time to verify the changes is the problem. The missing pieces are really just the extra stuff such as lavalink version, more filters (i haven't needed or used them), session info which isn't required, and i think route planning which i haven't needed either and quite frankly not sure how that all works. Unless someone else can help i won't be able to get that together.

That's okay, We can defer the v4 additions to another PR.

@LinuxDevon LinuxDevon marked this pull request as draft March 25, 2024 21:34
@LinuxDevon LinuxDevon marked this pull request as ready for review March 27, 2024 20:36
@LinuxDevon LinuxDevon requested a review from vilgotf March 27, 2024 20:36
@LinuxDevon
Copy link
Contributor Author

@vilgotf I think I have address all the open items. Please let me know if i missed anything or you want further changes. I appreciate the feedback!

Also what is up with the book and docs jobs failing? I didn't touch any of the other ones and it seems to be broken. Not sure if there is a fix for these or not?

@Erk-
Copy link
Member

Erk- commented Mar 28, 2024

Also what is up with the book and docs jobs failing? I didn't touch any of the other ones and it seems to be broken. Not sure if there is a fix for these or not?

The book stuff is maybe fixed by #2326, I don't really know whats up with the docs though.

Copy link
Member

@vilgotf vilgotf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better, but there's still some ways to go before landing this.

twilight-lavalink/Cargo.toml Outdated Show resolved Hide resolved
examples/lavalink-basic-bot.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/http.rs Show resolved Hide resolved
twilight-lavalink/src/http.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
twilight-lavalink/README.md Outdated Show resolved Hide resolved
twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/model/outgoing.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/model/incoming.rs Show resolved Hide resolved
twilight-lavalink/src/model/incoming.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/model/incoming.rs Outdated Show resolved Hide resolved
/// The guild ID of the player.
pub guild_id: Id<GuildMarker>,
/// Op code for this websocket event.
pub op: Opcode,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be an associated constant rather than part of the deserialized payload. Also, you can tag the IncomingEvent enum based on this field, which is what you appear to desire.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure i follow what you want here. See the example payload in the tests in the model.rs:

r#"{"op":"playerUpdate","guildId":"987654321","state":{"time":1710214147839,"position":534,"connected":true,"ping":0}}"#,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this in a follow up PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be done as part of this PR, don't think a follow-up would be merged any time soon seeing how we're progressing on this one...

Copy link
Member

@vilgotf vilgotf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for your work on this and sorry for the long wait. I pushed some changes (hope you don't mind) to help get this closer to the finish line. I don't expect to request any more changes after this.

twilight-lavalink/src/model/outgoing.rs Outdated Show resolved Hide resolved
twilight-lavalink/README.md Outdated Show resolved Hide resolved
fn connect_request(state: &NodeConfig) -> Result<ClientBuilder, NodeError> {
let mut builder = ClientBuilder::new()
.uri(&format!("ws://{}", state.address))
.uri(&format!("ws://{}/v4/websocket", state.address))
Copy link
Member

@Gelbpunkt Gelbpunkt Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the crate supports TLS, this should use wss conditionally. Considering you did not add TLS support for the HTTP part, I assume all of that is entirely untested if you left this as-is? Perhaps just remove TLS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see other comment about http2 support. Clarify that and I can then address this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gelbpunkt I looked into this a bit more. I was trying to enable tls support but this uri isn't allowed to be tls in the twilight-http. Should we just drop this implementation use and use the hyper client ourselves here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is twilight-http relevant at all here? This is the websocket connection part. Generally speaking though I think you won't be able to avoid forcing the user to specify whether TLS should be used for a particular node

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ClientBuilder is a struct used within the twilight http crate. I don't know why we have the socket connection using this crate and not using the hyper builder directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's a tokio-websockets struct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah sorry for the confusion... missed that. I was using my editor to find the definition and it matched the name of the twilight-http. Didn't double check the scope of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright so here is the deal. I did the rebase but the docs broke and can't reproduce locally. Also I have no idea how to make the TLS happen here. I am trying and running into all sorts of errors.

I get corrupted messages on the twilight side and and a bunch of invalid strings from the lavalink side. It is as if the websocket isn't upgrading to TLS correctly. I can't just simply change from ws to wss is what I am saying. I don't have time to debug this issue. I would like to make this fix later if possible as a issue ticket.

I think the prior behavior of this client was just on the ws not the upgraded TLS socket based on the diff.
.uri(&format!("ws://{}", state.address))

How would you like to proceed? I need help if we want this change in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a per-node parameter that toggles the use of TLS

twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved
twilight-lavalink/src/node.rs Show resolved Hide resolved
@@ -488,15 +509,18 @@ impl Connection {

let (to_node, from_lavalink) = mpsc::unbounded_channel();
let (to_lavalink, from_node) = mpsc::unbounded_channel();
let lavalink_http = HyperClient::builder(TokioExecutor::new()).build_http();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why keep TLS features and then make this plaintext-only? Also, why is there a HTTP2 feature? HTTP2 requires negotiation in the TLS handshake or prior knowledge, neither of which I see you doing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a change that @vilgotf did. That is why I was so confused what you were referring to.

Before:

let lavalink_http = HyperClient::builder(TokioExecutor::new())
            .http2_only(cfg!(feature = "http2"))
            .build_http();

#2322 (comment)

Please tell me what we want to do here. I would like to close this PR soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is enabled in the feature flags. The legacy client isn't clear but it is enabled here: https://docs.rs/hyper-util/latest/hyper_util/client/legacy/struct.Builder.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is still open but hinges on how to proceed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mention earlier, please add a TLS parameter to the Lavalink node that determines whether TLS (and therefore HTTP2) should be used.

@Gelbpunkt
Copy link
Member

The only major issue I have remaining is TLS and HTTP2. Either we support it, or we don't. But having TLS features for websockets which are unused, and HTTP2 features for HTTP which are unused is a no go. My wild guess would be that 99% of people run lavalink without TLS, so we can just remove the features? On the other hand, TLS support is perhaps like 10 lines of code, so it might be best to keep it (and properly implement it!).

@LinuxDevon
Copy link
Contributor Author

@vilgotf thank you for the help on updating the branch. Been super busy so haven't had a chance to respond here. I resolved your issues I believe.

@Gelbpunkt Can you be a bit more specific? I don't follow the comments. I think some of the confusion is from @vilgotf changes that were done. Please clarify how i should proceed. I would really like to close the PR out soon.

@LinuxDevon
Copy link
Contributor Author

@vilgotf and @Gelbpunkt I know it has been a bit but would like to close this. I don't have much time lately so haven't made any progress. What needs to happen to close this?

I tried to change the ws to wss as suggested and that is a bigger change than we think. That is using the twilight-http crate which apparently doesn't support tls yet? I tried to swap the uri and it won't connect to the server. Please let me know how you would like to proceed. If we can merge this we can open up an issue for fixing the TLS / HTTP 2 parts here. I just don't have time to implement / figure it out and test it.

The deadline for v3 support is coming to a close soon (November 1, 2024)

@Gelbpunkt
Copy link
Member

I think the best way forward now is to rebase the PR and look over all pending, unresolved review comments to see which ones still apply or need clarification. I haven't replied to all of them in time in the past, but I'm happy to revisit them now to get this merged.

Regarding TLS, twilight-http plays no part in the websocket connection. You will need to add a parameter at node creation for whether TLS should be used in order to decide the scheme to use for connecting.

Thanks again for trying to get this merged, it's a big PR which makes this a bit more troublesome for everyone involved, but I think we're approaching the goal soon.

LinuxDevon and others added 24 commits September 2, 2024 22:07
…e in the connection object to avoid continuously connecting / creating the client.
Updating some grammar errors, formatting, spelling, and some code reduction.
Authorization is to be redacted. In this case, the request is not that interesting so the entire event is removed
Added the ability to either include the identifier or the encoded track on and update. Each let you do different things for the server.
Copy link
Contributor Author

@LinuxDevon LinuxDevon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright so I took a look at getting the the websocket to use tls. This is not a straight forward change. The previous behavior is to us the non tls socket. So no behavioral changes happen. I would like to just make an issue ticket and move on with this. I know this isn't ideal but the leg work to get the upgraded socket is not straight forward an I do not have time.

I did as suggested and rebased. Everything seems to still be working in my bot that I am testing on. I resolved all the open issues that weren't resolved that should be closed now.

Please have another look. Thank you so much for the reviews and help! Sorry for being pushy and trying to close this off. Just limited on time and testing this is a bit of a setup and pain.

FYI: I don't know why the doc is failing. I can't reproduce locally on my branch.

fn connect_request(state: &NodeConfig) -> Result<ClientBuilder, NodeError> {
let mut builder = ClientBuilder::new()
.uri(&format!("ws://{}", state.address))
.uri(&format!("ws://{}/v4/websocket", state.address))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright so here is the deal. I did the rebase but the docs broke and can't reproduce locally. Also I have no idea how to make the TLS happen here. I am trying and running into all sorts of errors.

I get corrupted messages on the twilight side and and a bunch of invalid strings from the lavalink side. It is as if the websocket isn't upgrading to TLS correctly. I can't just simply change from ws to wss is what I am saying. I don't have time to debug this issue. I would like to make this fix later if possible as a issue ticket.

I think the prior behavior of this client was just on the ws not the upgraded TLS socket based on the diff.
.uri(&format!("ws://{}", state.address))

How would you like to proceed? I need help if we want this change in this PR.

@@ -488,15 +509,18 @@ impl Connection {

let (to_node, from_lavalink) = mpsc::unbounded_channel();
let (to_lavalink, from_node) = mpsc::unbounded_channel();
let lavalink_http = HyperClient::builder(TokioExecutor::new()).build_http();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is still open but hinges on how to proceed.

@@ -53,6 +57,7 @@ async fn main() -> anyhow::Result<()> {
let http = HttpClient::new(token.clone());
let user_id = http.current_user().await?.model().await?.id;

// The client is [`Lavalink`](crate::client::Lavalink) that forwards the required events from Discord.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice that you are trying to document what is going on here, but this is not a doc comment and won't get rendered anywhere. I'd try and rephrase it as a comment rather than documentation

Comment on lines +90 to +92
// We read the [Voice State and Voice Server Updates](https://discord.com/developers/docs/topics/gateway-events#voice) from discord to format the data to send to a Lavalink `VoiceUpdate` Event.
// There is a lower level [node](crate::node) that processes this for you. It isn't recommended to use this but rather the lavalink struct with the players. If you don't find functionality please open up and issue to expose what you need.
// See the command functions for where we use the players.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

default = ["http-support", "rustls-native-roots"]
http-support = ["dep:percent-encoding"]
default = ["http2", "rustls-native-roots"]
http2 = ["hyper/http2", "hyper-util/http2"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTTP2 as a separate feature makes little sense because it is pointless without prior knowledge, which I assume is virtually never the case. I'd add this implicitly to the TLS features.

Comment on lines +16 to +20
Currently some [Filters](crate::model::outgoing::Filters) are not yet supported.
Some endpoints such as [Lavalink Info] and [Update Session] have also not yet
been implemented. Please reach out and open an issue for any missing feature you
would like to use. The Lavalink V4 port did not add support for any new features
not previously found in V3.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this is supposed to go inside the changelog, not the README

/// Information about a playlist from a search result.
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
#[non_exhaustive]
#[serde(rename_all = "camelCase")]
pub struct PlaylistInfo {
/// The name of the playlist, if available.
pub name: Option<String>,
/// The name of the playlist
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The name of the playlist
/// The name of the playlist.

@@ -488,15 +509,18 @@ impl Connection {

let (to_node, from_lavalink) = mpsc::unbounded_channel();
let (to_lavalink, from_node) = mpsc::unbounded_channel();
let lavalink_http = HyperClient::builder(TokioExecutor::new()).build_http();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mention earlier, please add a TLS parameter to the Lavalink node that determines whether TLS (and therefore HTTP2) should be used.

};
}

tracing::error!("no session id is found. Session id should have been provided from the websocket connection already.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tracing::error!("no session id is found. Session id should have been provided from the websocket connection already.");
tracing::error!("no session id found");

let (method, url) = self.get_outgoing_endpoint_based_on_event(&outgoing)?;
let payload = serde_json::to_string(&outgoing).expect("serialization cannot fail");

let authority = url.authority().expect("Authority comes from endpoint");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let authority = url.authority().expect("Authority comes from endpoint");
let authority = url.authority().expect("authority comes from endpoint");

fn connect_request(state: &NodeConfig) -> Result<ClientBuilder, NodeError> {
let mut builder = ClientBuilder::new()
.uri(&format!("ws://{}", state.address))
.uri(&format!("ws://{}/v4/websocket", state.address))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a per-node parameter that toggles the use of TLS

@@ -652,9 +752,14 @@ async fn reconnect(
"key": config.address,
"timeout": resume.timeout,
});
let msg = Message::text(serde_json::to_string(&payload).unwrap());
let msg = Message::text(
serde_json::to_string(&payload).expect("Serialize can't panic here."),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
serde_json::to_string(&payload).expect("Serialize can't panic here."),
serde_json::to_string(&payload).expect("serialization cannot fail"),

@LinuxDevon
Copy link
Contributor Author

LinuxDevon commented Sep 7, 2024

I appreciate the feedback @Gelbpunkt. If this TLS / HTTP2 is a deal breaker then I am going to close this PR. I don't have time to make this work. As i stated, that isn't supported right now and this is just a port over to v4 api. The TLS issue is a feature request in my opinion. I simply don't have time to develop and test this huge change. It isn't just a simple switch URL and be done.

Someone else can come through and pick that up. I won't close the branch on my fork so anyone should be able to clone that and use this as a starting point.

@Gelbpunkt
Copy link
Member

It isn't just a simple switch URL and be done.

Yes, it is exactly just that. If you're keen on dropping the PR I can implement it as well, it's not a big deal.

@LinuxDevon
Copy link
Contributor Author

LinuxDevon commented Sep 7, 2024

If you have time to help implement that then please do. Once you get it to a point you are happy, let me know and I can give it a test on my bot. More than happy to have you just push to this branch.

I can run through the changes once we get to a good point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-lavalink Affects the lavalink crate m-breaking change Breaks the public API. t-feature Addition of a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants